Fabisev/artifact publishing #149

fabisev · 2025-08-19T16:14:24Z

Issue #, if available:

Description of changes:

Added build, test and publish workflow that uses Codebuild
Added copyright headers verifier and adder
Added automatic changelog generation
Added script for local testing

Target (OCI, Managed Runtime, both):
both

Checklist

I have run npm install to generate the package-lock.json correctly and included it in the PR.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…cript

trivenay · 2025-08-20T15:53:18Z

.github/workflows/build-and-release.yml

@@ -0,0 +1,135 @@
+name: Build and Release


I see the general flow of this is different from what we did

We are not using npmrc

We are not specific scope modifier --access=public

trivenay · 2025-08-20T15:53:44Z

.github/workflows/build-and-release.yml

+        if [[ "${{ steps.version.outputs.is_rc }}" == "true" ]]; then
+          npm publish aws-lambda-ric-*.tgz --tag rc
+        else
+          npm publish aws-lambda-ric-*.tgz


We publish for 2 arch -> We cannot use 1 tar file for it

trivenay · 2025-08-20T15:56:27Z

scripts/add-headers.js

@@ -0,0 +1,113 @@
+#!/usr/bin/env node


For adding things like Copyright header the most classic tool is git diff - It's market practise - Let's reuse that and not language specific tools - Good thought of adding this as part of linting, I hat when build/automated review fail due to this:P!

trivenay · 2025-08-20T15:57:15Z

scripts/generate-changelog.js

@@ -0,0 +1,97 @@
+#!/usr/bin/env node


Why do we need this if we are specifying executable before starting script

trivenay · 2025-08-20T15:59:38Z

scripts/test_local.sh

+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+set -e


For ease of debugging on this failing we can optionally do

set -euo pipefail trap 'echo "Error on line $LINENO"; exit 1' ERR

trivenay · 2025-08-20T16:02:01Z

scripts/test_local.sh

+local node_version=$1
+echo "Running unit tests for Node.js $node_version..."
+docker build -f test/unit/Dockerfile.nodejs${node_version}.x -t unit/nodejs.${node_version}x .
+docker run --rm unit/nodejs.${node_version}x


For checking failures

docker run … || { echo "Tests failed for Node.js $node_version"; exit 1; }

trivenay · 2025-08-20T16:02:55Z

scripts/test_local.sh

@@ -0,0 +1,36 @@
+#!/bin/bash


In general for formatting bash doesn't care but we need to add formatting for ease in readability

trivenay · 2025-08-20T16:03:44Z

scripts/test_local.sh

+"all")
+echo "Running unit tests for all Node versions..."
+for version in 18 20 22; do
+run_unit_tests $version


nit: Consider running them concurrently like command & => Only downside logs are interleaved which will make debugging hard

godcrampy · 2025-08-21T15:17:00Z

scripts/add-headers.sh

+      first_line=$(head -n1 "$file" 2>/dev/null || echo "")
+
+      # Create patch entry
+      echo "--- a/$file" >> "$patch_file"


Patch file itself could be created beforehand?

godcrampy · 2025-08-21T15:17:26Z

scripts/check-headers.sh

+      fi
+
+      # Create patch entry
+      echo "--- a/$file" >> "$patch_file"


Same question here, we don't need to create the patch file on the fly

merge main

maxday · 2025-08-26T07:21:31Z

.github/workflows/build-and-release.yml

+
+permissions:
+  id-token: write
+  contents: read


let's move this block to the job level according to this: https://github.com/ossf/scorecard/blob/ab2f6e92482462fe66246d9e32f642855a691dc1/docs/checks.md#token-permissions

maxday · 2025-08-26T07:22:37Z

.github/workflows/build-and-release.yml

+    - name: Setup Node.js
+      uses: actions/setup-node@v4
+      with:
+        node-version: '20'


let's use the latest LTS here?

maxday · 2025-08-26T07:24:33Z

.github/workflows/build-and-release.yml

+    - name: Setup Node.js
+      uses: actions/setup-node@v4
+      with:
+        node-version: '20'


same here, let's use latest LTS?

maxday · 2025-08-26T07:27:21Z

src/build.js

 };

-buildOneSet('node14.21.3');
+buildOneSet('node16.20.2');


why do we need node16 here?

maxday · 2025-08-26T07:27:37Z

package.json

@@ -18,7 +20,10 @@
    "format": "npm run format:src && npm run format:test",
    "format:src": "prettier --check \"src/*.*js\" --write",
    "format:test": "prettier --check \"test/**/*.*js\" --write",
-    "lint": "eslint --ext \".js\" src test",
+    "check-headers": "./scripts/check-headers.sh",


is the build failing if we have missing headers? Do we need to add this check-headers task in the build one?

maxday · 2025-08-26T07:28:46Z

README.md

+```shell script
+npm run add-headers
+```
+


thanks for updating the readme!

maxday · 2025-08-26T07:31:45Z

.github/workflows/build-and-release.yml

+          exit 1
+        fi
+        
+        # Copy architecture-specific package.json to dist


so we have 2 tars, one for each architecture?
At the end are we publishing one artifact to npm or 2?

maxday · 2025-08-26T07:32:52Z

.github/workflows/build-and-release.yml

+    needs: [get-version, build]
+    strategy:
+      matrix:
+        node-version: [18, 20, 22]


I think that's fine for now, but we might want to have one branch per version at some point if we want to adjust the RIC to a specific feature of node

maxday · 2025-08-26T07:34:34Z

.github/workflows/build-and-release.yml

+
+    - name: Setup NPM authentication
+      run: |
+        NPM_TOKEN=$(aws secretsmanager get-secret-value --secret-id aws-lambda-runtimes/github/nodejs/npm-token --query SecretString --output text)


I would add aws-lambda-runtimes/github/nodejs/npm-token as a github secret? not so secret but it would allow us to swap the secret name easily?

maxday · 2025-08-26T07:38:30Z

.github/workflows/build-and-release.yml

+
+    - name: Setup NPM authentication
+      run: |
+        NPM_TOKEN=$(aws secretsmanager get-secret-value --secret-id aws-lambda-runtimes/github/nodejs/npm-token --query SecretString --output text)


I think we need to make sure that this will never be triggered by untrusted PR.

So I think it would be better to split the build and release workflow. It will also simplify the checks around startsWith(github.ref, 'refs/tags/').

Maybe we can create a new workflow, manual_dispatch only with a input field which is the tag name when we want to release? WDYT? cc @godcrampy

Fabiana Severin added 21 commits July 11, 2025 19:40

Creating github actions workflow with publishing pipeline

51be34d

Adding Copyright headers, header checking script and header writing s…

88a021d

…cript

Adding rc publishing

98ab5eb

Commenting lint for now

5bdfeb3

Testing release

faaa65f

Adding automated changelog generation

1dc8f90

Refactoring automated changelog generation

f8c8ad5

Adding codebuild to the build

39ab54c

Adding the necessary dependencies for codebuild

5e88c22

Fixing build

a9d4d66

Fixing build

5a8defe

Checking npm publishing

86b1fe4

Changing the codebuild

cdc0773

Testing npm publishing using codebuild

9770fa8

Adding logs

0cae97e

Checking build on codebuild

eeecccb

Fixing build on codebuild

e69a19d

Fixing build on codebuild

3bcd7ea

Fixing build on codebuild

cad8992

Preparing github actions workflow for merge with main

c645473

Solving conflicts

201b966

trivenay reviewed Aug 20, 2025

View reviewed changes

fabisev marked this pull request as draft August 20, 2025 18:25

fabisev force-pushed the fabisev/artifact-publishing branch 7 times, most recently from 8d292b5 to 2e64bbd Compare August 20, 2025 19:52

Changing npm publishing

bdb6bf1

fabisev force-pushed the fabisev/artifact-publishing branch from 2e64bbd to bdb6bf1 Compare August 20, 2025 19:57

Fabiana Severin added 4 commits August 20, 2025 21:24

removing shebang line

331b87e

Adding echo for easier debugging and check if docker is avail

7e18147

Adding specific error message for different node versions and formatiing

d731168

Adding build and publishing for both architectures

523de7c

fabisev force-pushed the fabisev/artifact-publishing branch from d0e6613 to 523de7c Compare August 21, 2025 10:25

Making the add and check headers scripts use git diff and patches

160d714

fabisev force-pushed the fabisev/artifact-publishing branch from b3dfd05 to 160d714 Compare August 21, 2025 11:19

godcrampy reviewed Aug 21, 2025

View reviewed changes

Making headers scripts use patch files

595e7e8

fabisev force-pushed the fabisev/artifact-publishing branch from 3471e1c to 595e7e8 Compare August 21, 2025 16:23

godcrampy approved these changes Aug 22, 2025

View reviewed changes

fabisev force-pushed the fabisev/artifact-publishing branch from a763632 to 74c3b90 Compare August 22, 2025 14:26

Adding architecture specific tar

50265d9

fabisev force-pushed the fabisev/artifact-publishing branch 3 times, most recently from f4994c9 to 45f5f4e Compare August 25, 2025 09:51

FIxing build

61c7c17

fabisev force-pushed the fabisev/artifact-publishing branch from 45f5f4e to 61c7c17 Compare August 25, 2025 09:57

Merge pull request #155 from aws/main

d7955ae

merge main

maxday reviewed Aug 26, 2025

View reviewed changes

Fabisev/artifact publishing #149

Are you sure you want to change the base?

Fabisev/artifact publishing #149

Conversation

fabisev commented Aug 19, 2025

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!